Conversation
william051200
requested review from
jsntcy,
wangzelin007 and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
❌AzureCLI-BreakingChangeTest
Please submit your Breaking Change Pre-announcement ASAP if you haven't already. Please note:
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
There was a problem hiding this comment.
Pull request overview
This PR refactors parts of the VM command module by removing some output post-processing logic (identity/secrets) to simplify the code paths.
Changes:
- Simplifies
VMIdentityRemove._outputto directly return the deserialized output. - Simplifies
show_vm_identityto directly return the VMidentityobject (orNone). - Removes post-processing that injected
Nonevalues for certain missing/empty fields in identity and secret outputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/azure-cli/azure/cli/command_modules/vm/operations/vm.py |
Removes identity output normalization in VMIdentityRemove._output. |
src/azure-cli/azure/cli/command_modules/vm/custom.py |
Simplifies identity/secrets helpers and removes some output normalization logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def show_vm_identity(cmd, resource_group_name, vm_name): | ||
| vm = get_vm_by_aaz(cmd, resource_group_name, vm_name) | ||
|
|
||
| identity = vm.get("identity", {}) if vm else None | ||
|
|
||
| if identity and not identity.get('userAssignedIdentities'): | ||
| identity['userAssignedIdentities'] = None | ||
|
|
||
| return identity or None | ||
| return vm.get("identity") if vm else None |
There was a problem hiding this comment.
show_vm_identity no longer normalizes the identity payload: it can now return an empty dict (instead of None) when the VM has no identity, and it can omit/leave an empty userAssignedIdentities instead of returning null. This is observable in az vm identity show and breaks existing test expectations (e.g., checks for userAssignedIdentities == None and is_empty() for no identity). Please restore the previous output shape: return None when identity is missing/empty, and when identity exists but has no user-assigned identities, ensure userAssignedIdentities is present and set to None.
| if has_value(resource.type): | ||
| resource.type = AAZUndefined | ||
|
|
||
| result = self.deserialize_output(self.ctx.vars.instance, client_flatten=True) | ||
|
|
||
| identity = result.get('identity') | ||
| if not identity: | ||
| return result | ||
| return self.deserialize_output(self.ctx.vars.instance, client_flatten=True) | ||
|
|
There was a problem hiding this comment.
VMIdentityRemove._output previously ensured identity.userAssignedIdentities is serialized as null when empty/missing. Returning the raw deserialized output can change the CLI contract for identity removal (e.g., az vm identity remove output may omit userAssignedIdentities or return {}), which is inconsistent with other identity outputs that tests validate as None. Please keep the output normalization for identity.userAssignedIdentities (set to None when not present / empty) before returning the result.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Related command
Description
Refactored several lines of code to simplify future maintenance.
Testing Guide
History Notes
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.